[SDK] Feature: Makes chain optional on smart wallet config #5988
Conversation
🦋 Changeset detectedLatest commit: 0783395 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
size-limit report 📦
|
gregfromstl
force-pushed
the
greg/tool-3141-sdk-make-smart-account-chain-id-optional
branch
from
6027ff2 to
d1e7560
Compare
| const options = creationOptions; | ||
| const chain = connectChain ?? options.chain; | ||
| // Fallback to mainnet if no chain is provided (we only need this for pre-deploy signatures since transactions and deployments must define their own chain) | ||
| const chain = connectChain ?? options.chain ?? getCachedChain(1); |
There was a problem hiding this comment.
One issue I see is that below this we get the factory and account contracts based on this chain variable.
It works with our default factory because it's deterministic (though need to double check it's on mainnet) but might error if you pass another factory address and no chain.
Also would double check we don't use this chain car for other assumptions in this connect flow
gregfromstl
force-pushed
the
greg/tool-3141-sdk-make-smart-account-chain-id-optional
branch
from
7357730 to
cbdb7cd
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if ( | ||
| typeof createOptions.factoryAddress === "undefined" && | ||
| typeof createOptions.chain !== "undefined" | ||
| ) { | ||
| throw new Error("You must provide a chain if factory address is specified"); | ||
| } |
There was a problem hiding this comment.
The condition appears to be checking the wrong logic - it currently throws when factoryAddress is undefined but chain is defined. The intended validation is to require chain when factoryAddress is provided. The condition should be:
if (typeof createOptions.factoryAddress !== 'undefined' && typeof createOptions.chain === 'undefined')This ensures that a chain is specified whenever a custom factory address is used.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| if ( | ||
| typeof createOptions.factoryAddress === "undefined" && | ||
| typeof createOptions.chain !== "undefined" | ||
| ) { | ||
| throw new Error("You must provide a chain if factory address is specified"); | ||
| } |
There was a problem hiding this comment.
The validation logic appears to be inverted. The current check allows a chain to be specified without a factoryAddress, but throws when neither is provided. To enforce that a chain must be provided when factoryAddress is specified, the condition should be:
if (typeof createOptions.factoryAddress !== 'undefined' && typeof createOptions.chain === 'undefined')This ensures that smart wallets with custom factories have the required chain configuration.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
|
Closing as this breaks zk chains |
PR-Codex overview
This PR introduces changes to make the
chainparameter optional for smart wallet functionalities, enhancing flexibility in smart account integration. It also updates related tests and documentation to reflect these changes.Detailed summary
ConnectButtonto makechainoptional insmartWalletOptions.SmartWalletOptionsandSmartAccountOptionsto allow optionalchain.chainis undefined.